Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Faster optimized frozen dictionary creation (6/6) #88093

Merged
merged 8 commits into from
Jul 5, 2023
Merged

Faster optimized frozen dictionary creation (6/6) #88093

merged 8 commits into from
Jul 5, 2023

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Jun 27, 2023 •
edited
Loading

  • don't use custom ToArray for small frozen collections, up to 50% gain for creation time for collections with <= 4 items
Type Method Job Count Mean Ratio Allocated Alloc Ratio
Perf_Frozen<ReferenceType> ToFrozenDictionary PR 4 98.70 ns 0.54 152 B 0.66
Perf_Frozen<ReferenceType> ToFrozenDictionary main 4 182.50 ns 1.00 232 B 1.00
Perf_Frozen<ReferenceType> ToFrozenSet PR 4 165.18 ns 0.83 384 B 0.91
Perf_Frozen<ReferenceType> ToFrozenSet main 4 198.46 ns 1.00 424 B 1.00
  • use the int trick for uint, short, ushort, sbyte and byte: we know that all hash codes are unique and we can avoid some work later. 10-15% CPU time gain and 15-20% allocation reduction for FrozenDictionary and FrozenHashSet where TKey is uint, short, ushort, byte, sbyte
Method Job Count Mean Ratio Allocated Alloc Ratio
ToFrozenDictionary PR 64 2,808.38 ns 0.91 4400 B 0.78
ToFrozenDictionary main 64 3,072.64 ns 1.00 5656 B 1.00
ToFrozenSet PR 64 2,964.31 ns 0.87 5408 B 0.81
ToFrozenSet main 64 3,420.82 ns 1.00 6664 B 1.00
ToFrozenDictionary PR 512 25,362.06 ns 0.87 45216 B 0.84
ToFrozenDictionary main 512 28,944.59 ns 1.00 53672 B 1.00
ToFrozenSet PR 512 27,882.97 ns 0.84 51632 B 0.86
ToFrozenSet main 512 33,151.36 ns 1.00 60088 B 1.00
  • move Length Buckets code to a dedicated helper type to reduce code duplication and decrease code size

@stephentoub this is my last PR for frozen collections perf for now

I apologize if the CI gets red, for some reason I am getting a build error and I temporarily can't run unit tests locally:

CSC : error CS8785: Generator 'JSExportGenerator' failed to generate source. It will not contribute to the output a
nd compilation errors may occur as a result. Exception was of type 'TypeLoadException' with message 'Could not load
 type 'Microsoft.Interop.IGeneratorDiagnostics' from assembly 'Microsoft.Interop.SourceGeneration, Version=42.42.42
.42, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.' [D:\projects\runtime\src\libraries\System.Collections.Immu
table\src\System.Collections.Immutable.csproj::TargetFramework=net7.0]
CSC : error CS8785: Generator 'JSImportGenerator' failed to generate source. It will not contribute to the output a
nd compilation errors may occur as a result. Exception was of type 'TypeLoadException' with message 'Could not load
 type 'Microsoft.Interop.IGeneratorDiagnostics' from assembly 'Microsoft.Interop.SourceGeneration, Version=42.42.42
.42, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.' [D:\projects\runtime\src\libraries\System.Collections.Immu
table\src\System.Collections.Immutable.csproj::TargetFramework=net7.0]

contributes to #87964 and closes #87964 it as it's the last PR in the series

@adamsitnik
don't use custom ToArray for small frozen collections, up to 50% gain…
3973b70 
… for creation time for collections with <= 4 items
@adamsitnik
for these types GetHashCode returns their value casted to int, so whe…
3914d9b 
…n we receive a Dictionary/HashSet where there are key

we know that all hash codes are unique and we can avoid some work later

10-15% CPU time gain and 15-20% allocation reduction for FrozenDictionary and FrozenHashSet where TKey is uint, short, ushort, byte, sbyte
@adamsitnik
move Length Buckets code to a dedicated helper type to reduce code du…
0c29242 
…plication and decrease code size
@ghost ghost assigned adamsitnik Jun 27, 2023
@ghost
Copy link

ghost commented Jun 27, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details
  • don't use custom ToArray for small frozen collections, up to 50% gain for creation time for collections with <= 4 items
Type Method Job Count Mean Ratio Allocated Alloc Ratio
Perf_Frozen<ReferenceType> ToFrozenDictionary PR 4 98.70 ns 0.54 152 B 0.66
Perf_Frozen<ReferenceType> ToFrozenDictionary main 4 182.50 ns 1.00 232 B 1.00
Perf_Frozen<ReferenceType> ToFrozenSet PR 4 165.18 ns 0.83 384 B 0.91
Perf_Frozen<ReferenceType> ToFrozenSet main 4 198.46 ns 1.00 424 B 1.00
  • use the int trick for uint, short, ushort, sbyte and byte: we know that all hash codes are unique and we can avoid some work later. 10-15% CPU time gain and 15-20% allocation reduction for FrozenDictionary and FrozenHashSet where TKey is uint, short, ushort, byte, sbyte
Method Job Count Mean Ratio Allocated Alloc Ratio
ToFrozenDictionary PR 64 2,808.38 ns 0.91 4400 B 0.78
ToFrozenDictionary main 64 3,072.64 ns 1.00 5656 B 1.00
ToFrozenSet PR 64 2,964.31 ns 0.87 5408 B 0.81
ToFrozenSet main 64 3,420.82 ns 1.00 6664 B 1.00
ToFrozenDictionary PR 512 25,362.06 ns 0.87 45216 B 0.84
ToFrozenDictionary main 512 28,944.59 ns 1.00 53672 B 1.00
ToFrozenSet PR 512 27,882.97 ns 0.84 51632 B 0.86
ToFrozenSet main 512 33,151.36 ns 1.00 60088 B 1.00
  • move Length Buckets code to a dedicated helper type to reduce code duplication and decrease code size

@stephentoub this is my last PR for frozen collections perf for now

I apologize if the CI gets red, for some reason I am getting a build error and I temporarily can't run unit tests locally:

CSC : error CS8785: Generator 'JSExportGenerator' failed to generate source. It will not contribute to the output a
nd compilation errors may occur as a result. Exception was of type 'TypeLoadException' with message 'Could not load
 type 'Microsoft.Interop.IGeneratorDiagnostics' from assembly 'Microsoft.Interop.SourceGeneration, Version=42.42.42
.42, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.' [D:\projects\runtime\src\libraries\System.Collections.Immu
table\src\System.Collections.Immutable.csproj::TargetFramework=net7.0]
CSC : error CS8785: Generator 'JSImportGenerator' failed to generate source. It will not contribute to the output a
nd compilation errors may occur as a result. Exception was of type 'TypeLoadException' with message 'Could not load
 type 'Microsoft.Interop.IGeneratorDiagnostics' from assembly 'Microsoft.Interop.SourceGeneration, Version=42.42.42
.42, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.' [D:\projects\runtime\src\libraries\System.Collections.Immu
table\src\System.Collections.Immutable.csproj::TargetFramework=net7.0]
Author: adamsitnik
Assignees: -
Labels:

area-System.Collections, tenet-performance
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Jun 27, 2023
Closed
@jkoritzinsky
Copy link
Member

@adamsitnik what commit on main is your branch based on? I believe I fixed the build break you're seeing, so I want to make sure it didn't break in a different way.

@adamsitnik
Merge remote-tracking branch 'upstream/main' into minorFrozenPerfFixes
bdd93e6
@adamsitnik
address code review feedback:
29b7121 
* use nint and nuint rather than IntPtr and UIntPtr
* add tests for Frozen Dictionaries with key being uint, short, ushort, byte, sbyte
@adamsitnik
Copy link
Member Author

I believe I fixed the build break you're seeing, so I want to make sure it didn't break in a different way.

@jkoritzinsky I've synced my branch with upstream and everything works fine. Thanks for letting me know!

This was referenced Jun 28, 2023
Open
Closed
Closed
@adamsitnik adamsitnik mentioned this pull request Jun 29, 2023
Open
@build-analysis build-analysis bot mentioned this pull request Jun 29, 2023
Closed
@adamsitnik
address code review feedback: add nint and uint test coverage
4fbdc78 
and also fix a discovered bug: IntPtr started implementing IComparable<IntPtr> in .NET 5
stephentoub
stephentoub reviewed Jul 4, 2023
View reviewed changes
src/libraries/System.Collections.Immutable/tests/Frozen/FrozenDictionaryTests.cs Outdated
byte[] bytes = new byte[8];
random.NextBytes(bytes);
return BitConverter.ToInt64(bytes, 0);
#endif
Copy link
Member

@stephentoub stephentoub Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not going to matter for testing purposes, but technically these aren't the same thing. NextInt64(long.MinValue, long.MaxValue) will never produce long.MaxValue, but the else code can. For simplicity you might want to just delete the NET6_0_OR_GREATER case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will never produce long.MaxValue, but the else code can.

Good point! After thinking about it I decided to remove the NET6_0_OR_GREATER case as it could cause another kind of issues: generate different output for the same input and hence get us to a situation where exactly the same test is passing for one moniker and failing for another.

This was referenced Jul 4, 2023
Closed
Closed
@adamsitnik
Copy link
Member Author

The failure is unrelated (#87477), merging.

@adamsitnik adamsitnik merged commit 76a8f4f into dotnet:main Jul 5, 2023
@eerhardt eerhardt mentioned this pull request Jul 10, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gfoidl gfoidl gfoidl left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees

@adamsitnik adamsitnik

Labels
area-System.Collections tenet-performance Performance related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Frozen collection construction performance
4 participants
@adamsitnik @jkoritzinsky @stephentoub @gfoidl